feat(types): Add TBF header serialization support with set_flags function#109
feat(types): Add TBF header serialization support with set_flags function#109thoguii wants to merge 4 commits into
Conversation
| match self { | ||
| TbfHeader::TbfHeaderV2(hd) => { | ||
| let base = &hd.base; | ||
| let mut bytes = [0u8; 16]; | ||
| bytes[0..2].copy_from_slice(&base.version.to_le_bytes()); | ||
| bytes[2..4].copy_from_slice(&base.header_size.to_le_bytes()); | ||
| bytes[4..8].copy_from_slice(&base.total_size.to_le_bytes()); | ||
| bytes[8..12].copy_from_slice(&base.flags.to_le_bytes()); | ||
| bytes[12..16].copy_from_slice(&base.checksum.to_le_bytes()); | ||
| Ok(bytes) | ||
| } | ||
| TbfHeader::Padding(base) => { | ||
| let mut bytes = [0u8; 16]; | ||
| bytes[0..2].copy_from_slice(&base.version.to_le_bytes()); | ||
| bytes[2..4].copy_from_slice(&base.header_size.to_le_bytes()); | ||
| bytes[4..8].copy_from_slice(&base.total_size.to_le_bytes()); | ||
| bytes[8..12].copy_from_slice(&base.flags.to_le_bytes()); | ||
| bytes[12..16].copy_from_slice(&base.checksum.to_le_bytes()); | ||
| Ok(bytes) | ||
| } |
There was a problem hiding this comment.
Refactor suggestion:
let base = match self {
TbfHeader::TbfHeaderV2(hd) => &hd.base,
TbfHeader::Pading(base) => &base,
};
let bytes = ....
| } | ||
| } | ||
|
|
||
| pub fn update_from_serialize(&mut self, bytes: &[u8]) -> Result<(), TbfParseError> { |
There was a problem hiding this comment.
What does this function do?
There was a problem hiding this comment.
Thanks for the feedback! I did this function to provide a way to sync the Tbf_base with externally modified bytes without the need to re-parse. It just parses bytes back into struct, tho indeed it's better to use parse_tbf_header directly.
| } | ||
| } | ||
|
|
||
| self.serialize() |
There was a problem hiding this comment.
I wouldn't return anything for this function
eva-cosma
left a comment
eva-cosma
left a comment
There was a problem hiding this comment.
Looks really good! Two observations though:
- Could you write some documentation for the pub fn functions you wrote?
- Could write some tests for your functions?
feat(tests/serialization): add tests for TBF header serialization
There was a problem hiding this comment.
I appreciate the enthusiasm with which you write the tests, but I feel like some of these overlap with eacother, or multiple could be merged into a single test.
I did not comment all of the tests, I'm going to leave it as an exercise for you to try to merge as many / deduplicate them.
| } | ||
|
|
||
| #[test] | ||
| fn footer_sha256() { |
There was a problem hiding this comment.
Since we're essentialy just testing the serialization of the first 16 bytes, having a footer does not make this test any different from the simple_tbf test
| // Serialization | ||
|
|
||
| #[test] | ||
| fn simple_tbf() { |
There was a problem hiding this comment.
| fn simple_tbf() { | |
| fn serialize_identical_with_original() { |
| } | ||
|
|
||
| #[test] | ||
| fn footer_rsa4096() { |
There was a problem hiding this comment.
Same as above, footer does not affect header base serialization
|
|
||
| pub fn set_flags(&mut self, flags: u32, header: &[u8]) -> Result<[u8; 16], TbfParseError> { | ||
| /// Sets the flag field and updates the checksum, it modifies the state accordingly | ||
| pub fn set_flags(&mut self, flags: u32, header: &[u8]) -> Result<(), TbfParseError> { |
There was a problem hiding this comment.
Besides this function, I would create a
- set_enabled
- set_sticky
and anything else the flags can be. Just so that the end-user of this crate does not need to know which bit is the 'enabled' or 'sticky' bit
| let mut header = parse_tbf_header(&buffer[0..header_len as usize], 2).unwrap(); | ||
|
|
||
| assert!(header.enabled()); | ||
| assert_eq!(header.get_package_name().unwrap(), "_heart"); |
There was a problem hiding this comment.
package name not relevant for this test
| } | ||
|
|
||
| #[test] | ||
| fn enable_disable_footer_sha256() { |
There was a problem hiding this comment.
Same as above, footer does not affect header base serialization
| } | ||
|
|
||
| #[test] | ||
| fn padding_header2() { |
There was a problem hiding this comment.
| fn padding_header2() { | |
| fn padding_header_set_flags() { |
fix(tests/serialization): removed redundant tests and make enable and sticky tests explicit
2 participants
Pull Request Overview
TODO or Help Wanted
Checks
Using Rust tooling
cargo fmtcargo clippycargo testcargo buildFeatures tested:
GitHub Issue
This pull request closes #117